Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IDisposableAnnotations #130

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

IDisposableAnnotations #130

wants to merge 8 commits into from

Conversation

JohanLarsson
Copy link
Collaborator

Fixes #126


public interface IWithAnnotations
{
[return:Dispose]
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DisposedByCaller is longer but perhaps clearer?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. Would MustDispose be clearer (while still short)?

/// The containing method owns the instance and is responsible for disposing it.
/// </summary>
[AttributeUsage(AttributeTargets.Parameter, AllowMultiple = false, Inherited = true)]
public class DisposesAttribute : Attribute
Copy link

@jnm2 jnm2 Nov 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To come up with an alternative for the sake of discussion: [BecomesOwner]? [TransferOwnership]?

[Disposes] sounds like the method itself does the disposing. Is that always the case or might it be saved for the class to dispose later?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to make it a verb to be more explicit for the callers. MustBeDisposed and NotDisposable?

/// The return value should be disposed by caller.
/// </summary>
[AttributeUsage(AttributeTargets.ReturnValue, AllowMultiple = false, Inherited = true)]
public class DisposeAttribute : Attribute
Copy link

@jnm2 jnm2 Nov 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just spaghetti: [OwnedByCaller]? [CallerOwns]?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having Disposable somewhere in the name is good.

/// The return value should not be disposed by caller.
/// </summary>
[AttributeUsage(AttributeTargets.ReturnValue, AllowMultiple = false, Inherited = true)]
public class DontDisposeAttribute : Attribute
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the contraction. [NotOwnedByCaller]?

@JohanLarsson JohanLarsson changed the title DisposableAnnotations IDisposableAnnotations Nov 5, 2018
/// The return value should not be disposed by caller.
/// </summary>
[AttributeUsage(AttributeTargets.ReturnValue, AllowMultiple = false, Inherited = true)]
public class DontDisposeAttribute : Attribute

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd spell it out. DoNotDispose 😉

/// The return value should not be disposed by caller.
/// </summary>
[AttributeUsage(AttributeTargets.ReturnValue | AttributeTargets.Parameter, AllowMultiple = false, Inherited = true)]
public class DonNotDisposeAttribute : Attribute

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One too many n there. ;-)

using System;

/// <summary>
/// The return value should be disposed by caller.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably better if we change should to must to be consistent with the attribute's name.

@bclothier
Copy link

Any progress on this? We would love to see this in action.

@bclothier
Copy link

I want to check and see if this will be coming soon? This is a blocking issue for me.

@JohanLarsson
Copy link
Collaborator Author

I don't really remember where we left this but we discussed that it would be nasty if we require the attribute dll to be shipped with applications or the attribute package a package dependency for libraries.

We also discussed opening an issue about adding the attributes to the framework but I don't remember what happened with that.

@jeremyVignelles
Copy link

@JohanLarsson : I asked the question on their gitter, but didn't have any official answer. I have never taken time to ask on the repo

@bclothier
Copy link

The way I see it if we dont want to ship attributes, this leaves us with two alternatives:

  1. we overload some existing attribute. For instance, use the DataContract and DataMember and assign a special meaning to a XML namespace.

  2. make use of comments, perhaps even custom elements in the xmldocs. IINM, analyzers can still get the comments, so it can be used without needing an attribute class.

@jnm2
Copy link

jnm2 commented Jan 27, 2019

JetBrains' annotations package puts [Conditional("JETBRAINS_ANNOTATIONS")] on every attribute: https://www.fuget.org/packages/JetBrains.Annotations/2018.3.0/lib/netstandard2.0/JetBrains.Annotations.dll/JetBrains.Annotations/PureAttribute?code=true'

This means that analyzers can see the annotations by using syntax even though the attributes are never emitted at compile time (unless you define the symbol JETBRAINS_ANNOTATIONS).

If you add such a NuGet package, you can also include a .targets that keeps the attributes assembly from being copied to build output. I wrote a build trimming tool, so I can help with that.

@jeremyVignelles
Copy link

I finally asked the question there : https://github.com/dotnet/coreclr/issues/24668

@bclothier
Copy link

What is the current blocking issue here?

@JohanLarsson
Copy link
Collaborator Author

Not sure there is anything blocking, did not really like the design and have not had much time.

@jnm2
Copy link

jnm2 commented Oct 12, 2019

Now that the C# nullability attributes have shipped, I wonder if a similar naming convention could be used.

@JohanLarsson
Copy link
Collaborator Author

What would the names be then?

@jnm2
Copy link

jnm2 commented Oct 12, 2019

https://docs.microsoft.com/en-us/dotnet/api/system.diagnostics.codeanalysis?view=netcore-3.0
Actually, they seem less helpful than I was thinking. We can't use Disposable in place of Null because disposing is never optional. You either must or must not dispose, right? There's no "may dispose" for the caller and there's no "might dispose" for the callee.

Taking the concept of directing some at inputs (AllowNull, DisallowNull) and others at outputs (NotNull, MaybeNull), here's a straw man based on the ideas above:

MustDispose, MustNotDispose: applies to the caller's subsequent use of the value

WillDispose, WillNotDispose: signals whether or not the operation is transferring ownership away from the caller.

If and When variants could follow just like in System.Diagnostics.CodeAnalysis if they prove useful.

@jnm2
Copy link

jnm2 commented Oct 12, 2019

I'd actually like to see something like this added to the framework and understood by Roslyn's and fxcop's dispose analyzers.

@jeremyVignelles
Copy link

You either must or must not dispose, right? There's no "may dispose" for the caller and there's no "might dispose" for the callee.

I came across the case once where I wondered if it was mandatory to dispoed, because I think I read a code in asp.net core where it isn't dispoed:

https://docs.microsoft.com/en-us/dotnet/api/microsoft.extensions.primitives.ichangetoken.registerchangecallback?view=aspnetcore-3.0

The purpose of the IDisposable here is to say "I don't want to be notified anymore". If the registration is meant to live as long as the application, is the Dispose() really mandatory?

I'd actually like to see something like this added to the framework and understood by Roslyn's and fxcop's dispose analyzers.

There does not seem to be a big progress here : https://github.com/dotnet/corefx/issues/37873 . I've seen they put a milestone on that but...

@Bouke
Copy link
Contributor

Bouke commented Apr 8, 2020

It would be great if we could annotate our APIs with ownership information. Besides our own code, it would be nice if we could somehow annotate third party code as well. See for example StreamReader which is currently hard-coded in the analyzer, but what about other classes -- we can't expect this analyzer to hard-code every third-party API out there.

I agree with the point posted here that we should avoid having to ship a dll in our release builds. However the annotations should be clear as well, I'm not sure how such a thing can be achieved.

For completeness we would need a way to annotate leaveOpen (optional ownership). Maybe something like TakesOwnershipWhenAttribute(string, bool), e.g.:

public StreamReader(
    [TakesOwnershipWhen("leaveOpen", false)] Stream stream,
    bool leaveOpen)
{ }

/// The ownership of instance is transferred and the receiver is responsible for disposing.
/// </summary>
[AttributeUsage(AttributeTargets.Parameter, AllowMultiple = false, Inherited = true)]
public class TakesOwnershipAttribute : Attribute
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AraHaan
Copy link

AraHaan commented Dec 6, 2021

After thinking about this, what if the analyzer contained a source generator that would generate the annotation codes that people can use to control the analyzer part of itself?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Annotations for cases the analyzer can't figure out.
7 participants